-
Notifications
You must be signed in to change notification settings - Fork 168
chore: Bump version manually only in pyproject.toml #2514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I can definitly replicate the issue locally, but I hoped that it was some env issue π₯² Edit: changing the following: - entry: python -m utils.check_api_reference
+ entry: python utils/check_api_reference.pyleads to
so yes, narwhals is not installed π€ |
|
@FBruzzesi I wonder if the new |
@dangotbanned this PR was aiming at something else, but we could definitly replace: narwhals/utils/bump_version.py Lines 45 to 57 in 42813b4
with subprocess.run(["uv", "version", "--bump", how])π€© |
Mentioned in (#2514 (comment)) I'm guessing the other scripts will need to be updated as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @FBruzzesi π₯³
| ] | ||
|
|
||
|
|
||
| def __getattr__(name: _t.Literal["__version__"]) -> str: # type: ignore[misc] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be completely honest I'd rather just update in two places, I have no idea what this is doing
Happy to take the installation update in the docs page though π
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure I understand correctly - the concern with importing importlib.metadata is regarding startup time? see comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i feel uneasy about the global - do you have a reference from another project that does this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking for __version__ = version(__name__) on github you will get many results
A couple of projects I know that made it in the first few pages of the list are:
- connector-x
- manim
- patito (in a try-except block)
Edit: some other project do the analogous of __version__ = version("narwhals") (but couldn't find something I know)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, thanks
still not sure we need it though - there's some other nice improvements in here though, happy to take those
regarding perf, from narwhals import __version__ currently takes 0m0.156s, and from importlib import metadata takes 0m0.108s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some notes:
__version__doesn't seem to be mentioned as a standard in the python docs- I understand lots of packages seem to provide it anyway, but I'm not sure where the pattern is documented
__getattr__is a well-defined method of deferring imports- There are going to be many more examples of this pattern following
3.14
IMO, I don't think there's a use-case for checking narwhals.__version__, but if people want to do that - the cost of it isn't paid by everyone else since we're providing it lazily.
Are there any search results of a package checking it? (sorry I'm replying from my phone π
)
@MarcoGorelli I understand your caution and ultimately will defer π to you, but I think this balances:
- Theoretical backwards-compatibility
- Avoids introducing performance overhead for those that don't need it
- Ensures that potential cost is only paid once
- Lowers our maintenance burden, by simplifying version bumps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks - could you elaborate on the global part please? i've had a look at the linked projects and they don't seem to have it
if we don't deem __version__ to be useful to most users, presumably we don't need the global either?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks - could you elaborate on the
globalpart please? i've had a look at the linked projects and they don't seem to have it
Sure @MarcoGorelli!
I'm assuming you're referring to the projects in (#2514 (comment))?
The 3 stdlib examples I gave in (#2514 (comment)) all featured use of global:
- https://github.com/python/cpython/pull/132060/files
- https://github.com/python/cpython/pull/128898/files
- https://github.com/python/cpython/pull/129860/files
I guess for some more info on global, these might be helpful
- https://docs.python.org/3/tutorial/classes.html#python-scopes-and-namespaces
- https://docs.python.org/3/tutorial/classes.html#scopes-and-namespaces-example
- https://docs.python.org/3/reference/simple_stmts.html#global
- https://docs.python.org/3/reference/datamodel.html#modules
- https://docs.python.org/3/reference/datamodel.html#module.__dict__
As a more concrete example, we can do the following on main:
import narwhals as nw
>>> nw.__version__
'1.38.2'
>>> "__version__" in nw.__dict__
True
>>> nw.__dict__["__version__"]
'1.38.2'
>>> nw.__version__ == nw.__dict__["__version__"] == nw.__dict__.__getitem__("__version__")
TrueBut in this PR, we can see where the __getattr__ hook slips in:
import narwhals as nw
>>> nw.__dict__["__version__"]
KeyError: '__version__'
>>> nw.__version__
'1.38.2'
>>> nw.__dict__["__version__"]
'1.38.2'Without the global:
__version__is never added tonw.__dict__- Each lookup of
nw.__version__always goes via__getattr__, which is triggered after the regular__dict__lookup fails
So the global is what provides the lazy import, since the work is only ever done once and only when a user asks for it π.
All subsequent lookups behave identically to a regular lookup into the namespace.
As another example, polars has 2 behaviors related to __getattr__:
- Deprecations are triggered on every lookup
- E.g.
pl.NUMERIC_DTYPES
- E.g.
- Dependencies use much more magic than I understand π, but they eventually resolve to replacing a module's globals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, polars seems to be fine with using find_spec and still considers that lazy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

What type of PR is this? (check all applicable)
Related issues
Checklist